Prohibit wildcard import in pylint #8893
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8893 +/- ##
========================================
Coverage 73.84% 73.84%
========================================
Files 412 412
Lines 44261 44261
Branches 3993 3993
========================================
Hits 32686 32686
Misses 11575 11575
|
|
I agree with the policy, but we do have extant uses of wildcard imports in the codebase: $ rg 'import *' -F -t py --sort=path
cvat/apps/engine/__init__.py
7:from .schema import * # force import of declared symbols
cvat/settings/development.py
5:from .base import *
cvat/settings/email_settings.py
6:from cvat.settings.production import *
cvat/settings/production.py
5:from .base import *
cvat/settings/testing.py
5:from .development import *
cvat/settings/testing_rest.py
5:from cvat.settings.production import *
cvat-sdk/cvat_sdk/models.py
5:from cvat_sdk.api_client.models import * # pylint: disable=unused-import,redefined-builtin
serverless/pytorch/foolwood/siammask/nuclio/model_handler.py
5:from tools.test import *
tests/python/cli/conftest.py
5:from sdk.fixtures import * # pylint: disable=unused-import
tests/python/conftest.py
5:from shared.fixtures.data import *
6:from shared.fixtures.init import *
7:from shared.fixtures.util import *
tests/python/sdk/conftest.py
5:from .fixtures import *Would you mind going through these and fixing/suppressing the warning? |
|
Ok, it seems that these uses just reexport symbols or do some other technical stuff. |
Sure, but you still have to suppress the warning. I'm also pretty sure that the star import in |
|
|
||
|
|
||
| class ModelHandler: | ||
| def __init__(self): | ||
| # Setup device | ||
| self.device = torch.device('cuda' if torch.cuda.is_available() else 'cpu') | ||
| torch.backends.cudnn.benchmark = True | ||
| self.device = impl.torch.device('cuda' if impl.torch.cuda.is_available() else 'cpu') |
There was a problem hiding this comment.
torch is just PyTorch, so could you import it normally (import torch)?
There was a problem hiding this comment.
I didn't want to change any logic in this script.
|
Motivation and context
In python, wildcard imports like
from x import *can unpredictably pollute namespace with symbols that are not really expected. In most cases it's better to useimport x as yto get access to all module symbols. For popular libraries importable contents of modules are typically maintained well, but it requires quite a lot of work. Currently, we don't use wildcard imports for anything in the project and I can't currently think of places where it could really be useful, so I think it's best to just prohibit their use entirely.How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit